[#1063] Give back what the open reserved rather than what the configuration says by then, and ask for a restart when the cache size changes - #1066

praise: The quota now follows the cache that actually runs, instead of the configuration as it stands at close.

• close() gives back reservedCacheSize in both PDBStorage and JEStorage, and buildConfiguration finally honours the result of acquireMemory (PDBStorage.java:1101, JEStorage.java:778).
• computeSize() reads serverContext.getMemoryQuota() rather than the memQuota field, which is null until the first open.
• The restart is declared in two places: component-restart in both XMLs, and adminActionRequired plus NOTE 630 in the ConfigChangeResult.

issue (blocking): After the quota refuses the open's reservation, isConfigurationChangeAcceptable refuses every change to the backend entry, including a change that leaves the cache size alone.

opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1556, opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1286

A refused reservation leaves reservedCacheSize == 0 while the storage stays open. From then on, newSize <= reservedCacheSize is false for every change. The check falls through to isMemoryAvailable(newSize), which asks for the same amount the quota just refused, and it adds no reason. ConfigurationHandler.replaceEntry asks every change listener on the entry, and nothing filters by property (ConfigurationHandler.java:620-627, ConfigChangeListenerAdaptor.java:339-345). So the following all fail with UNWILLING_TO_PERFORM and an empty reason:

• a db-txn-no-sync change;
• dsconfig set-backend-prop --set enabled:false;
• the internal ds-cfg-enabled: false modify that TaskUtils.disableBackend makes for an online import-ldif, rebuild-index or restore (TaskUtils.java:203-210).

BASE accepted all of these through newSize <= computeSize(config). Startup opens are not checked against the quota, so this state needs no config change. It comes up with a third PDB/JE backend at the default db-cache-percent 50. It also comes up with one JE backend whose db-cache-size is more than half the reservable pool, because validateDbCacheSize has already taken that size and kept it (#1067). A server restart gets back to the same state.

    final long newSize = computeSize(newCfg);
    final MemoryQuota quota = serverContext.getMemoryQuota();
    // What does not grow past the size already configured asks the quota for nothing (BASE's rule);
    // a growth is measured against what this storage holds, which is what the next open adds to.
    return (newSize <= Math.max(reservedCacheSize, computeSize(config))
            || quota.isMemoryAvailable(newSize - reservedCacheSize))
        && checkConfigurationDirectories(newCfg, unacceptableReasons);

aCacheSizeChangeIsAdmittedAgainstWhatTheStorageHolds keeps its outcome under this fix: with 64 MB held and 128 MB pending, 256 MB is refused and 192 MB admitted at 129 MB free.

Pin: repeat the open of aReservationTheQuotaRefusedIsNotGivenBackOnClose (32 MB free, a 64 MB cache), then assert that a db-txn-no-sync-only change is acceptable. Do this in both classes. The assertion is red at this head.

    final PDBBackendCfg unchangedCache = createBackendCfg(SMALL_CACHE);
    when(unchangedCache.isDBTxnNoSync()).thenReturn(true);
    assertThat(storage.isConfigurationChangeAcceptable(unchangedCache, new ArrayList<LocalizableMessage>())).isTrue();

suggestion (non-blocking): Admission is only asserted where the reservation equals the configured size. A refused open, an unchanged size and a shrink are never checked.

opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java:638, :663, opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/JEStorageTest.java:311, :334

The only calls to isConfigurationChangeAcceptable come after a granted 64 MB open, where reservedCacheSize == configuredCacheSize. Two mutants survive all six cases in both classes (found by reading; not run):

• reservedCacheSize replaced by configuredCacheSize in the admission line;
• the newSize <= reservedCacheSize || short circuit dropped. In production that sends every shrink to Semaphore.tryAcquire(negative), which throws IllegalArgumentException.

The blocking issue above ships green for the same reason.

Pin: after the refused open of aReservationTheQuotaRefusedIsNotGivenBackOnClose, assert that createBackendCfg(SMALL_CACHE + SMALL_CACHE / 4) is not acceptable. It is 16 MB above what is configured but 80 MB above what is held, which kills the reserved → configured swap. Then, after a granted open with the quota drained to 0 free, assert that createBackendCfg(SMALL_CACHE / 2) is acceptable. That kills the dropped short circuit.

suggestion (non-blocking): The percent arm of the restart note is not pinned. Every case sizes the cache with db-cache-size.

opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java:616

Each applyConfigurationChange call in the class (:570, :586, :604, :624, :644) uses createBackendCfg(n * SMALL_CACHE). None goes through db-cache-percent, which is the shipped default (size 0, percent 50). The mutant newCacheSize = cfg.getDBCacheSize() at PDBStorage.java:1649 survives. On a percent-sized backend, that mutant makes every unrelated change set adminActionRequired and emit a note saying "X bytes … 0 bytes".

Pin: open with createBackendCfg(0) and getDBCachePercent() stubbed to 10. Assert that a db-txn-no-sync-only change at percent 10 leaves adminActionRequired() false, and that a change to percent 20 sets it and names memPercentToBytes(10) and memPercentToBytes(20).

suggestion (non-blocking): No test shows that the "opened with" baseline stays fixed across two changes.

opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java:598

Every restart-note case applies a single change to a storage that was just opened. So the mutant configuredCacheSize = newCacheSize; after the addMessage survives. In production, that mutant turns open 64 → change 128 → change back to 64 into a restart note for a pool that still runs at 64.

    final ConfigChangeResult back = storage.applyConfigurationChange(createBackendCfg(SMALL_CACHE));
    assertThat(back.adminActionRequired()).isFalse();
    assertThat(back.getMessages()).isEmpty();

Pin: append this to aCacheSizeChangedWhileOpenAsksForARestart in both classes.

suggestion (non-blocking): The open guard of the restart note (db != null / env != null) is not pinned.

opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/JEStorageTest.java:271, opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1650, opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1380

Every case that calls applyConfigurationChange opens the storage first. The mutant that drops the guard survives both classes. On a storage that has been constructed but never opened, the listener is already registered, and the mutant sets adminActionRequired and names "0 bytes opened with". The effect is small: the apply then fails in registerMonitoredDirectory anyway, at BASE as at this head.

Pin: construct a storage without opening it, apply a config with a different cache size, and assert that adminActionRequired() is false and that NOTE 630 is not among the messages.

issue (non-blocking): NOTE 630 calls the opened-with size "reserved", including when the quota refused the reservation.

opendj-server-legacy/src/messages/org/opends/messages/backend.properties:1173, opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1657, opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1387

Both callers pass configuredCacheSize into "the memory reserved for it … stay at the %d bytes". After a refused open the storage holds 0 bytes of the quota. A later cache change that the quota now admits therefore reports memory as reserved when none is. On the granted road the text is accurate.

NOTE_CONFIG_DB_CACHE_REQUIRES_RESTART_630=The change to the database cache of backend %s will not take effect \
 until the backend is restarted: the cache the backend runs with stays at the %d bytes it was opened with until \
 then, and the %d bytes now configured are reserved by the next open

praise: A refused reservation no longer turns into a refused disable, and the quota now follows what the open actually took.

• PDBStorage.isConfigurationChangeAcceptable:1559-1560 / JEStorage.isConfigurationChangeAcceptable:1469-1470 admit a change that does not grow the size past Math.max(reservedCacheSize, computeSize(config)) without asking the quota. Only a growth is measured against what the storage holds. aChangeWhichLeavesTheCacheSizeAloneIsAdmittedAfterARefusedReservation pins this in both classes.
• close() gives back reservedCacheSize (PDBStorage:1137, JEStorage:891), and a refused acquireMemory is now recorded as nothing held (PDBStorage:1101, JEStorage:842).

issue (non-blocking): For a JE backend sized by db-cache-percent, NOTE 630 prints the quota's estimate as "the cache the backend runs with", which is not the cache JE actually runs.

opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:839, :1563-1571; opendj-server-legacy/src/messages/org/opends/messages/backend.properties:1173-1175; opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/ConfigurableEnvironment.java:295

configuredCacheSize is memQuota.memPercentToBytes(percent), a percent of the quota's reservable pool ((e/π)² × Old Gen max). JE does not use that number. It sizes its own cache from je.maxMemoryPercent = db-cache-percent, which is a percent of Runtime.maxMemory. Take the default JE backend (size 0, percent 50) on -Xmx2g under G1: JE runs about 1 GiB, and a change to percent 40 reports that the cache "stays at the ~803,000,000 bytes it was opened with". On the percent arm the printed figure is always below JE's real cache. PDB builds its pool from configuredCacheSize (PDBStorage:1098), so PDB and a fixed-size JE backend report correctly. The quota arithmetic itself is consistent. For JE's percent arm, either name the configured percentage in the note, or describe the figure as the size the memory quota was asked for.

suggestion (non-blocking): No test covers the reservedCacheSize operand of the admission's Math.max.

opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1559, opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1469

In every admission call in both classes, max(reserved, config) == config. The mutant newSize <= computeSize(config) || …, which is BASE's shape, therefore stays green. aCacheShrunkWhileOpenIsGivenBackAsItWasTaken is the one case with reserved > config, and it closes without asking for admission. Under that mutant, a sequence of open at 128 MB, shrink to 64 MB, then change to 96 MB calls isMemoryAvailable(-32 MB). That goes to Semaphore.tryAcquire(-32) (MemoryQuota:86, :103), which throws IllegalArgumentException out of the change listener.

@Test
public void aGrowthWithinWhatIsHeldAfterAShrinkAsksTheQuotaForNothing() throws Exception
{
  closeAndRemove(storage);
  storage = new PDBStorage(createBackendCfg(2 * SMALL_CACHE), serverContext);
  storage.open(AccessMode.READ_WRITE);
  storage.applyConfigurationChange(createBackendCfg(SMALL_CACHE));

  assertThat(storage.isConfigurationChangeAcceptable(
      createBackendCfg(SMALL_CACHE + SMALL_CACHE / 2), new ArrayList<LocalizableMessage>())).isTrue();
}

Pin: the same case in JEStorageTest. Both are green at the head, and both throw IllegalArgumentException under the mutant.

suggestion (non-blocking): The restart note is asserted only after a growth, so a shrink while open could lose it with every case still green.

opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1654, opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1564; opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java:594, opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/JEStorageTest.java:289

Every NOTE 630 assertion follows either a growth (SMALL_CACHE → 2 * SMALL_CACHE, percent 10 → 20) or a return to the opened size. The one shrink in each class discards its ConfigChangeResult. So the mutant newCacheSize > configuredCacheSize passes both classes. Under it, a live shrink returns SUCCESS with no admin action while the cache keeps its opened size.

// aCacheShrunkWhileOpenIsGivenBackAsItWasTaken
final ConfigChangeResult ccr = storage.applyConfigurationChange(createBackendCfg(SMALL_CACHE));
assertThat(ccr.adminActionRequired()).isTrue();
assertThat(ccr.getMessages().get(0).toString()).isEqualTo(
    NOTE_CONFIG_DB_CACHE_REQUIRES_RESTART.get("PDBStorageTest", 2 * SMALL_CACHE, SMALL_CACHE).toString());

Pin: the same in JEStorageTest, using BACKEND_ID. It is red under != → >.

suggestion (non-blocking): The move from the memQuota field to serverContext.getMemoryQuota() is not pinned for a storage that is not yet open.

opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1558, :1564-1568; opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1468, :1474-1478

memQuota is null from the constructor, which already registers the listener, until buildConfiguration runs. Every admission call in the tests is on an open storage, where the field and serverContext.getMemoryQuota() are the same instance. The one unopened case uses a fixed SMALL_CACHE, so computeSize never reaches the quota. If either method went back to memQuota, both classes would stay green. In production, that version would throw a NullPointerException for a percent-sized backend (the default) changed between BackendImpl.configureBackend:196 and openBackend:204.

@Test
public void aStorageWhichIsNotOpenAdmitsAChangeOfItsCachePercent() throws Exception
{
  final PDBStorage unopened = new PDBStorage(createBackendCfg(0L, 10), serverContext);
  try
  {
    assertThat(unopened.isConfigurationChangeAcceptable(
        createBackendCfg(0L, 20), new ArrayList<LocalizableMessage>())).isTrue();
  }
  finally
  {
    unopened.close();
  }
}

Pin: the same case in JEStorageTest. It is green at the head and throws NullPointerException if either method reads memQuota again.

nitpick (non-blocking): The javadoc of aStorageWhichIsNotOpenAsksForNoRestart says the change "is picked up by the open", but the apply fails before config = cfg.

opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java:653, opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/JEStorageTest.java:349-350

On a storage that was never opened, diskMonitor is still null, so registerMonitoredDirectory throws a NullPointerException. The catch turns it into an error result, and the next open uses the old configuration. The case still pins the open guard, which is what it is for. Reword the javadoc to say that a storage which is not open asks for no restart.

praise: Round 2's points are all answered in both storages, and each one is pinned.

• NOTE 630 now describes both figures as what the memory quota counts (backend.properties:1173-1175). The javadoc of JEStorage.configuredCacheSize (:736-738) explains why, on the percent arm, that figure differs from the cache JE actually runs.
• Every round-2 test point now has a case in both classes. aGrowthWithinWhatIsHeldAfterAShrinkAsksTheQuotaForNothing (PDBStorageTest:795, JEStorageTest:490) covers the reservedCacheSize operand of the admission's Math.max. aStorageWhichIsNotOpenAdmitsAChangeOfItsCachePercent (PDBStorageTest:814, JEStorageTest:509) covers reading the quota through serverContext. aCacheShrunkWhileOpenIsGivenBackAsItWasTaken now asserts NOTE 630 after a shrink (PDBStorageTest:599, JEStorageTest:294).